-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CSI Hostpath Driver & VolumeSnapshots addons #8461
CSI Hostpath Driver & VolumeSnapshots addons #8461
Conversation
Hi @11janci. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Can one of the admins verify this patch? |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 11janci The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #8461 +/- ##
=======================================
Coverage 34.05% 34.05%
=======================================
Files 153 153
Lines 9840 9840
=======================================
Hits 3351 3351
Misses 6086 6086
Partials 403 403 |
/assign @josedonizetti |
d2c8b8f
to
6d175ba
Compare
/assign @medyagh |
6d175ba
to
19b8123
Compare
19b8123
to
fbb2341
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @11janci thank you for your contribution! There are two things I'd like to add to this PR:
- Documentation around what this addon does and how to use it
- An integration test to validate the addon works
You can write the documentation as volume-snapshot.md
here: https://github.com/kubernetes/minikube/tree/master/site/content/en/docs/tutorials
For the integration test --
The test would:
- Start a minikube cluster, and enable the addon by adding the
--addons=<addon name>
flag (Sample code here) - Makes sure the installation was successful (as in, some expected pod came up as
Running
) (sample code here) - Try to snapshot a volume and make sure it worked as expected
You can add TestVolumeSnapshotAddon
to our addons_test.go file.
A lot of the code from TestAddons can be copied into TestVolumeSnapshotAddon
(I linked some key lines above).
Docs for quickly running an integration test locally can be found here. Please let me know if you have any questions!
@@ -415,10 +415,42 @@ var Addons = map[string]*Addon{ | |||
MustBinAsset( | |||
"deploy/addons/ambassador/ambassadorinstallation.yaml", | |||
vmpath.GuestAddonsDir, | |||
"ambassadorinstallation.yaml.yaml", | |||
"ambassadorinstallation.yaml", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch!
fbb2341
to
3d1f666
Compare
3d1f666
to
7e4bcc7
Compare
810d62c
to
abceb71
Compare
@priyawadhwa thank you for your comments, the PR is now ready for review. Pls see the updated description |
abceb71
to
aa80902
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the integration test! I've left a few small nits to fix. I'll kick off the integration tests and let's see how they do.
/ok-to-test |
@priyawadhwa thanks for all the comments! I will fix the small issues right away.
Basically the only reason was that it is enabled by default in standard k8s as well, so I thought I'll mirror that. I'll be happy to disable it if you'd prefer that. And lastly, I see bunch of tests failed but they should be all unrelated. The new test passed. |
aa80902
to
804b8aa
Compare
Hey @11janci yah, let's disable by default then! Also, if both of those addons need to be enabled for them to work, I'd add a warning in as a validation. Something like:
|
kvm2 Driver Times for Minikube (PR 8461): 88.4s 89.1s 85.8s Averages Time Per Log
docker Driver Times for Minikube (PR 8461): 55.4s 54.8s 44.9s Averages Time Per Log
|
fa19319
to
bad98aa
Compare
Travis tests have failedHey @11janci, 1st Buildmake test
TravisBuddy Request Identifier: 727408c0-eec0-11ea-9705-b7c481b3600a |
bad98aa
to
b54e3b2
Compare
Travis tests have failedHey @11janci, 1st Buildmake test
TravisBuddy Request Identifier: 7128cea0-eec1-11ea-9705-b7c481b3600a |
b54e3b2
to
78d135e
Compare
Travis tests have failedHey @11janci, 1st Buildmake test
TravisBuddy Request Identifier: 3a464010-eec2-11ea-9705-b7c481b3600a |
Travis tests have failedHey @11janci, 1st Buildmake test
TravisBuddy Request Identifier: adc44a50-eec2-11ea-9705-b7c481b3600a |
78d135e
to
b696eb6
Compare
@priyawadhwa all done & ready for review 🙂 |
Thanks for this contribution! |
Fixes #8343
Fixes #8815
Adds the csi-hostpath-driver and volumesnapshots addons to minikube. Splitting it into two separate addons gives the users the flexibility to configure the CSI driver of their choice (should they prefer a different driver than CSI Hostpath), without having to set up volume snapshots by themselves.
The change is split into three logical commits - it might be easier to review it as such: